-
Notifications
You must be signed in to change notification settings - Fork 3
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Refactor wasm codes #9
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i love this design! nice i think this is pretty close.
there are a few patterns i think could be cleaned up.
- i don't see a huge point into separating
WasmCodeUpdater
fromWasmCodeService
. it feels like the service should just be able to handle everything. maybe it can have two new functions that are likestartUpdatingFromDB
andstopUpdatingFromDB
, and the start function can be called automatically when thenewWithWasmCodesFromDB
service creator is used. - since it's becoming a service, i expect it to be removed from config entirely instead of adding the service to the config object. i like the idea of adding the updater's singleton pattern to the service itself, and instead of using
config.wasmCodes?...
everywhere, usingWasmCodeService.getInstance()...
. i thinkconfig
should remain just the serializable data loaded from the file. - no need to update every 2 seconds. changes will be quite rare. maybe 60 seconds?
- let's move this to a new services folder in
src/services/wasm-codes
instead of right in thesrc
folder. it's becoming a bit unruly.
thanks so much for doing this. you are clearly very capable :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
good changes. i think there's some more cleanup necessary but the service is pretty stable
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
sick!!!! just one more thing 😁
and there's one more conversation left up there unresolved ^
Looking delicious! |
can you rebase on my latest changes from main to fix the merge conflicts? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
awesome!!!!!!!
1- Move wasm codes to db
2- Reload the codes every 2 seconds(temp)
3- Encapsulate some of the codeIds ops